Trac #768 - Add GEOSSTRtree_nearest to CAPI #61

dbaston · 2016-02-13T22:54:00Z

https://trac.osgeo.org/geos/ticket/768

This is a necessary precursor to implementing MinimumClearance in GEOS/PostGIS, but useful in its own right.

dbaston · 2016-02-13T22:55:52Z

@strk how does one run the GEOS unit tests through valgrind? When I intentionally introduce a memory leak, it doesn't show up with valgrind --trace-children=yes ctest.

strk · 2016-02-15T10:04:47Z

I had no idea you could use "ctest" (I dont' use cmake).
What I do is running the specific test I want to run via libtool,
example:

 cd tests/unit
 libtool --mode=execute valgrind ./geos_unit

dbaston · 2016-02-15T13:59:07Z

Thanks, that did the trick.

dbaston · 2016-04-13T23:16:53Z

@strk any thoughts about this pull request?

mloskot · 2016-04-14T10:36:19Z

@dbaston Since there is no MemCheck config in the CMakeLists.txt, you would have to patch it using CTEST_MEMORYCHECK_* variables, then it would be possible to use CTest MemCheck.

strk · 2016-04-14T12:31:36Z

Could you add some documentation about the new CAPI entry ?
A few lines in a comment above the signature in the public
header file would do.

Also, can we avoid the void pointers in the distance function ?

dbaston · 2016-04-14T13:10:32Z

Yes, some documentation is clearly needed. The idea with the void pointers (and the purpose of a user providing their own distance function) is because we don't know the type of the objects stored in the STRtree (PostGIS stores LWGEOMS in them, for example). This is similar to the use of void pointers in the user-provided callback to GEOSSTRtree_query. If the distance function is NULL, it assumes a distance between GEOS geometries.

Looking at this again after quite a bit of time away, I can see that the way I exposed this flexibility in the CAPI is flawed. There's currently no way you could query a tree of LWGEOMS. I think the signature actually needs to be:

extern const void* GEOS_DLL GEOSSTRtree_nearest(
    GEOSSTRtree *tree,
    const GEOSGeometry* envelope,
    const void* item,
    int (*distancefn)(const void* item1, const void* item2, double* distance));

Is there a better way to do it?

strk · 2016-04-14T15:49:12Z

On Thu, Apr 14, 2016 at 06:10:32AM -0700, Dan Baston wrote:

extern const void* GEOS_DLL GEOSSTRtree_nearest(
GEOSSTRtree tree,
const GEOSGeometry envelope,
const void* item,
int (distancefn)(const void item1, const void* item2, double* distance));

Until I see the documentation I can't tell if there's a better way to
do it :)

Note that I didn't implement the GEOSSTRtree interface myself so that
part is also obscure to me. I guess the tree stores pairs of
"envelopes" and associated arbitrary pointers, and only ever uses
those envelopes to do its operations, is that correct ?

In your suggested signature, is the item parameter the one that
would be passed as first parameter to the distancefn ?
Any reason it should be named "item" if that's the case, rather
than just being an arbitrary "context" for the caller ?
(it could for example also store the GEOS context).

dbaston · 2016-04-14T16:04:32Z

Yes, item would be passed as the first parameter to distancefn. Perhaps the signature of distancefn could be changed to take something more general, but I tried not to stray from the JTS implementation. Here's a crack at some documentation:

/*
 * Returns the nearest item in the STRtree to the supplied item
 *
 * @param tree the STRtree to search
 * @param item the item with which the tree should be queried
 * @param itemGeom a GEOSGeometry having the bounding box of 'item'.  If 'item' is
 *            itself a GEOSGeometry, it may be provided again here
 * @param distancefn a function that can compute the distance between two items
 *            in the STRtree.  The function should return nonzero in case of error,
 *            and should store the computed distance to the location pointed to by
 *            the 'distance' argument.  If NULL, the default GEOS geometry distance 
 *            function will be used.
 * 
 * @return a const pointer to the nearest item in the tree to 'item', or NULL in
 *            case of exception
 */
extern const void* GEOS_DLL GEOSSTRtree_nearest(GEOSSTRtree *tree,
                                        const void* item,
                                        const GEOSGeometry* itemEnvelope,
                                        int (*distancefn)(const void* item1, const void* item2, double* distance));

dbaston · 2016-04-14T16:06:08Z

I guess the tree stores pairs of "envelopes" and associated arbitrary pointers, and only ever uses
those envelopes to do its operations, is that correct ?

Yes. It's similar to the JTS implementation, which stores Object.

strk · 2016-04-14T16:55:19Z

On Thu, Apr 14, 2016 at 09:04:33AM -0700, Dan Baston wrote:

Yes, item would be passed as the first parameter to distancefn. Perhaps the signature of distancefn could be changed to take something more general, but I tried not to stray from the JTS implementation. Here's a crack at some documentation:
/*
 * Returns the nearest item in the STRtree to the supplied item
 *
 * @param tree the STRtree to search
 * @param item the item with which the tree should be queried
 * @param itemGeom a GEOSGeometry having the bounding box of 'item'.  If 'item' is
 *            itself a GEOSGeometry, it may be provided again here

You meant "itemEnvelope" here, I guess.
"May" or "Must" be provided again ?

@param distancefn a function that can compute the distance between two items

       in the STRtree.  The function should return nonzero in case of error,

       and should store the computed distance to the location pointed to by

       the 'distance' argument.  If NULL, the default GEOS geometry distance

```
       function will be used.
```

Will the default GEOS geometry distance assume that the void pointer
is in effect a GEOSGeometry ?
Should the provided function guarantee an invariant based on the
envelope of each item in the tree ? (like: the distance between the
envelopes of two items cannot be bigger than the distance returned
by the distance function, or something along those lines).

@return a const pointer to the nearest item in the tree to 'item', or NULL in
       case of exception

Could the function be extended to return the N nearest items ?

dbaston · 2016-04-14T17:24:00Z

You meant "itemEnvelope" here, I guess.

Yes

"May" or "Must" be provided again ?

Let's say "must", but I think a better solution is to have two signatures (see below)

Will the default GEOS geometry distance assume that the void pointer
is in effect a GEOSGeometry ?

Yes. Again, resolved by having two signatures.

Should the provided function guarantee an invariant based on the
envelope of each item in the tree ?

Presumably, but I've never seen it documented what that invariant is. I've never used anything other than a plan-old distance function

Could the function be extended to return the N nearest items ?

Unfortunately not, I think that change would have to happen at the JTS level.

Since this is proving to be a bit cumbersome in the most likely case, where the tree is in fact storing GEOSGeometry objects, maybe there should be two functions:

/*
 * Returns the nearest item in the STRtree to the supplied GEOSGeometry
 *
 * @param tree the STRtree to search
 * @param geom the geometry with which the tree should be queried
 * @return a const pointer to the nearest GEOSGeometry in the tree to 'geom', or NULL in
 *            case of exception
 */
extern const GEOSGeometry* GEOS_DLL GEOSSTRtree_nearest(GEOSSTRtree *tree, const GEOSGeometry* geom);

/*
 * Returns the nearest item in the STRtree to the supplied item
 *
 * @param tree the STRtree to search
 * @param item the item with which the tree should be queried
 * @param itemEnvelope a GEOSGeometry having the bounding box of 'item'
 * @param distancefn a function that can compute the distance between two items
 *            in the STRtree.  The function should return nonzero in case of error,
 *            and should store the computed distance to the location pointed to by
 *            the 'distance' argument.
 * @return a const pointer to the nearest item in the tree to 'item', or NULL in
 *            case of exception
 */
extern const void* GEOS_DLL GEOSSTRtree_nearest_generic(GEOSSTRtree *tree,
                                        const void* item,
                                        const GEOSGeometry* itemEnvelope,
                                        int (*distancefn)(const void* item1, const void* item2, double* distance));

strk · 2016-04-15T07:32:13Z

/*
Returns the nearest item in the STRtree to the supplied GEOSGeometry
*

@param tree the STRtree to search

@param geom the geometry with which the tree should be queried

@return a const pointer to the nearest GEOSGeometry in the tree to 'geom', or NULL in
       case of exception
/
extern const GEOSGeometry GEOS_DLL GEOSSTRtree_nearest_generic(GEOSSTRtree tree, const GEOSGeometry geom);

An empty tree would then throw an exception ? Or the input geometry ?
The function name, I guess, would be GEOSSTRtree_nearest_geometry ?

/*

Returns the nearest item in the STRtree to the supplied item
*

@param tree the STRtree to search

@param item the item with which the tree should be queried

@param itemEnvelope a GEOSGeometry having the bounding box of 'item'

I'd say that "itemEnvelop" is the object with which the tree is
queried, while "item" (or some other name) is an opaque object
that is passed to the distancefn callback function for each of
the "candidate" (to be better explained?) in the tree ?

@param distancefn a function that can compute the distance between two items
       in the STRtree.  The function should return nonzero in case of error,
       and should store the computed distance to the location pointed to by
       the 'distance' argument.
@return a const pointer to the nearest item in the tree to 'item', or NULL in
       case of exception
/
extern const void GEOS_DLL GEOSSTRtree_nearest_generic(GEOSSTRtree tree,
const void item,
const GEOSGeometry* itemEnvelope,
int (distancefn)(const void item1, const void* item2, double* distance));

dbaston · 2016-04-15T13:14:32Z

An empty tree would then throw an exception ? Or the input geometry ?

Yes, an empty tree or input geometry should throw an exception and return NULL. I will add test cases for this.

I'd say that "itemEnvelop" is the object with which the tree is
queried, while "item" (or some other name) is an opaque object
that is passed to the distancefn callback function for each of
the "candidate" (to be better explained?) in the tree ?

I don't want to provide too much detail on how the function is executed; the important thing (from a caller's perspective) is that it computes the distance between objects of the type that is stored in the tree, and with which the tree is queried. This is just coming from the JTS interface ItemDistance:

    /**
     * Computes the distance between two items.
     *
     * @param item1
     * @param item2
     * @return the distance between the items
     *
     * @throws IllegalArgumentException if the metric is not applicable to the arguments
     */

I will change the CAPI to have both GEOSSTRtree_nearest and GEOSSTRtree_nearest_generic, and then I'll add a test case using GEOSSTRtree_nearest_generic with a user-defined geometry type. Sound good?

strk · 2016-04-15T13:33:44Z

On Fri, Apr 15, 2016 at 06:14:32AM -0700, Dan Baston wrote: I don't want to provide too much detail on how the function is executed; the important thing (from a caller's perspective) is that it computes the distance between objects of the type that is stored in the tree, and with which the tree is queried.

In that case the function would need another parameter to hold a context. I mean, while in Java/C++ you can have "functor", in C you need an explicit parameter to know from where the distance is called. There could be different reasons to have a context, for example a pointer to a custom error reporting function or memory allocator or whatever. So, if if I know the first parameter to my provided callback will always be the pointer I passed to the function, then I can use that pointer to hold whatever context I want.

dbaston · 2016-04-15T23:53:47Z

I'm not understanding the need to provide a context in this particular case, but I'm no expert here and very happy to take your word for it. That said, I don't want to thread a void pointer all the way through this algorithm, breaking similarity with JTS, for what's probably a fringe use case. How about we just forget about supporting a "nearest" operation on anything other than GEOSGeometry? That leaves us with simply

/*
 * Returns the nearest item in the STRtree to the supplied GEOSGeometry
 *
 * @param tree the STRtree to search
 * @param geom the geometry with which the tree should be queried
 * @return a const pointer to the nearest GEOSGeometry in the tree to 'geom', or NULL in
 *            case of exception
 */
extern const GEOSGeometry* GEOS_DLL GEOSSTRtree_nearest(GEOSSTRtree *tree, const GEOSGeometry* geom);

strk · 2016-04-16T10:27:35Z

So I went looking at the GEOSSTRtree interface to see what we have now. I think it is good to be consistent. There's a query function that takes a callback and passes a "userdata" void pointer to the callback: typedef void (*GEOSQueryCallback)(void *item, void *userdata); That callback type is used for both GEOSSTRtree_query and GEOSSTRtree_iterate, wheras both functions take the userdata pointer as an argument (and pass it to the callback verbatim). Unfortunately there's no unit test for the GEOSSTRtree functions, nor for the underlying STRtree C++ class. It would be useful to start there to understand the interface better. I think to understand that all GEOSSTRtree functions exposed at the C-API level only deal with indexing objects by their envelopes, leaving it to the caller to tell them apart (not trying to extract an envelope from an item). And they return items whose reported envelopes intersect the envelope used for querying. Now the "nearest" function is adding handling an additional object, which is the item itself. And probably assuming something about the relation between item distance and item envelope distance. I find in the code that items are grouped togheter in groups "boundables" based on envelope proximity. Finding the "nearest" object is probably done based on envelope distance first, avoiding to look inside nodes whose envelope distance is bigger than a candidate distance found, and updating the candidate distance to the closest distance within the node to the query item. So I guess the invariant is that no item distance can ever be smaller than item envelope distance. Sorry if this analisys is confusing, as I actually hoped for it to help in further shaping the API. Given you are currently allowed to insert opaque objects in the index (associated to a GEOSGeometry envelope) I do find it consistent to also allow using an opaque function to compute item distance (as long as it is documented). Even more so than assuming the item is a geometry.

dbaston · 2016-04-16T11:05:24Z

@strk thanks for the review and analysis. This matches my understanding of the API too. (I'm happy to suggest some docstring comments for the rest of the STRtree functions too, after this is wrapped up).

Looking at this some more, it looks like we can pass void* userdata to the user-provided distance function without "contaminating" the ItemDistance class, as I had thought before. So we can do the two signatures, adding another parameter to the GEOSSTRtree_nearest_generic version:

/*
 * Returns the nearest item in the STRtree to the supplied GEOSGeometry
 *
 * @param tree the STRtree to search
 * @param geom the geometry with which the tree should be queried
 * @return a const pointer to the nearest GEOSGeometry in the tree to 'geom', or NULL in
 *            case of exception
 */
extern const GEOSGeometry* GEOS_DLL GEOSSTRtree_nearest(GEOSSTRtree *tree, const GEOSGeometry* geom);

/*
 * Returns the nearest item in the STRtree to the supplied item
 *
 * @param tree the STRtree to search
 * @param item the item with which the tree should be queried
 * @param itemEnvelope a GEOSGeometry having the bounding box of 'item'
 * @param distancefn a function that can compute the distance between two items
 *            in the STRtree.  The function should return nonzero in case of error,
 *            and should store the computed distance to the location pointed to by
 *            the 'distance' argument.
 * @param userdata optional pointer to arbitrary data; will be passed to distancefn
 *            each time it is called.
 * @return a const pointer to the nearest item in the tree to 'item', or NULL in
 *            case of exception
 */
extern const void* GEOS_DLL GEOSSTRtree_nearest_generic(GEOSSTRtree *tree,
                                                        const void* item,
                                                        const GEOSGeometry* itemEnvelope,
                                                        int (*distancefn)(const void* item1, const void* item2, double* distance, void* userdata),
                                                        void* userdata);

strk · 2016-04-17T17:07:32Z

Given the insert function does not make any assumption about the inserted object being a geometry, I don't see making the assumption from the nearest-query function as consistent. How about just keping the custom-distance one ? Also please add the invariant expectation for the distance function.

…tancefn

dbaston · 2016-04-18T21:06:02Z

OK, I've modified the PR to:

add Doxygen comments for the GEOSSTRtree_nearest functions and all other functions having to do with STRtrees
correctly handle a nearest query on an empty tree
add unit tests showing the use of GEOSSTRtree_nearest with a non-GEOS data type
allow user to pass an arbitrary void pointer to distancefn

The motivation for having a GEOSGeometry-specific version of the "nearest" function was that the generic version is a bit confusing....for a GEOSGeometry you'd have to call it as GEOSSTRtree_nearest(tree, g, g, NULL, NULL). I'm fine with taking it out though if you prefer.

strk · 2016-04-19T08:49:36Z

From the documentation of the generic function it doesn't look like you can pass NULL as the distance function, right ? So to use a geometry-distance function you should still pass in your own distance function.

If you want to keep the geometry-specific function you should document very carefully that it will assume that the items put in the STRtree objects are in fact geometries.

Please also add tests for passing empty geometries (this one would be also interesting to do for the tree insertion functions, like passing empty envelope).

Any reason not to use a typedef for the callback function ? As it is used for other callbacks, it would sound more consistent.

dbaston · 2016-04-19T13:26:50Z

If you want to keep the geometry-specific function you should document very carefully that it will assume that the items put in the STRtree objects are in fact geometries.

Added a note to the docstring.

Please also add tests for passing empty geometries (this one would be also interesting to do for the tree insertion functions, like passing empty envelope).

Added.

Any reason not to use a typedef for the callback function ? As it is used for other callbacks, it would sound more consistent.

I remember that I found the typedef of GEOSQueryCallback confusing (as opposed to seeing the expected function signature) when I first used used this pat of the API a while back, but I think having the function signature described in the docstring mitigates this. I added a typedef for GEOSDistanceCallback.

strk · 2016-04-19T14:12:05Z

Getting closer, I built and tested and all looked fine, but after squash-rebasing to svn-trunk and taking a final look I found this: ``` + try + { + if (distancefn) { + struct CustomItemDistance : public ItemDistance { + CustomItemDistance(GEOSDistanceCallback p_distancefn, void* p_userdata) + : m_distancefn(p_distancefn), m_userdata(p_userdata) {} + + ``` Is that not c++11 only, to define a structure in a conditional block ?

dbaston · 2016-04-19T15:43:53Z

My (very limited) understanding is that it only requires C++11 if you want to use it as a template parameter.
http://stackoverflow.com/questions/10979984/are-there-any-penalties-for-defining-a-struct-inside-a-function

strk · 2016-04-19T16:01:21Z

Indeed building with -std=c++98 seems ok. I'll push as-is, then maybe move the definition outside the try block

strk · 2016-04-19T16:06:58Z

Merged as r4184 = 63d2554 (refs/remotes/trunk)
Thanks

dbaston · 2016-04-19T16:09:41Z

Excellent - thanks for working with me on this!

strk · 2016-04-19T16:46:17Z

Thank you ! Looking forward for your next PR, maybe on Gogs this time :)

Includes tests for the new API and pre-existing STRtree API Closes #768 Patch by Daniel Baston <dbaston@gmail.com> via #61 git-svn-id: http://svn.osgeo.org/geos/trunk@4184 5242fede-7e19-0410-aef8-94bd7d2200fb

strk · 2016-11-10T07:32:44Z

The PR was incomplete, can you look at it @dbaston ?
See https://trac.osgeo.org/geos/ticket/796

initial work for GEOSSTRtree_nearest.

dffba3d

dbaston force-pushed the minclearance branch from 9fea140 to dffba3d Compare April 13, 2016 22:58

dbaston added 4 commits April 18, 2016 16:42

Revise CAPI so that we can now provide a void* context argment to dis…

6c290c4

…tancefn

Avoid null dereference when querying nearest item in empty tree

d90f144

Add additional tests for GEOSSTRtree_nearest

19d8c4a

whoops, guess we're not using C++11 yet

319fff7

dbaston added 2 commits April 19, 2016 09:09

Add tests for GEOSSTRtree_nearest in trees containing empty geometries

8ab214c

add typedef for GEOSDistanceCallback

724a1b2

strk closed this Apr 19, 2016

strk pushed a commit that referenced this pull request Feb 7, 2019

Disable cmake perf tests (#61)

91ed20d

brendan-ward mentioned this pull request Mar 15, 2020

PROPOSAL: optimize STRtree for geometry collections / multipart geometries pygeos/pygeos#112

Closed

tomalrussell mentioned this pull request Aug 13, 2021

ENH: Expose interface to sindex.nearest (#1977) geopandas/geopandas#2053

Merged

pramsey mentioned this pull request Nov 14, 2022

3.11.1: test suite is failing in test_docs unit #734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trac #768 - Add GEOSSTRtree_nearest to CAPI #61

Trac #768 - Add GEOSSTRtree_nearest to CAPI #61

dbaston commented Feb 13, 2016

dbaston commented Feb 13, 2016

strk commented Feb 15, 2016

dbaston commented Feb 15, 2016

dbaston commented Apr 13, 2016

mloskot commented Apr 14, 2016

strk commented Apr 14, 2016

dbaston commented Apr 14, 2016

strk commented Apr 14, 2016

dbaston commented Apr 14, 2016

dbaston commented Apr 14, 2016

strk commented Apr 14, 2016

dbaston commented Apr 14, 2016 •

edited

strk commented Apr 15, 2016

dbaston commented Apr 15, 2016

strk commented Apr 15, 2016 via email

dbaston commented Apr 15, 2016

strk commented Apr 16, 2016 via email

dbaston commented Apr 16, 2016

strk commented Apr 17, 2016 via email

dbaston commented Apr 18, 2016

strk commented Apr 19, 2016

dbaston commented Apr 19, 2016

strk commented Apr 19, 2016 via email

dbaston commented Apr 19, 2016

strk commented Apr 19, 2016 via email

strk commented Apr 19, 2016

dbaston commented Apr 19, 2016

strk commented Apr 19, 2016 via email

strk commented Nov 10, 2016

Trac #768 - Add GEOSSTRtree_nearest to CAPI #61

Trac #768 - Add GEOSSTRtree_nearest to CAPI #61

Conversation

dbaston commented Feb 13, 2016

dbaston commented Feb 13, 2016

strk commented Feb 15, 2016

dbaston commented Feb 15, 2016

dbaston commented Apr 13, 2016

mloskot commented Apr 14, 2016

strk commented Apr 14, 2016

dbaston commented Apr 14, 2016

strk commented Apr 14, 2016

dbaston commented Apr 14, 2016

dbaston commented Apr 14, 2016

strk commented Apr 14, 2016

dbaston commented Apr 14, 2016 • edited

strk commented Apr 15, 2016

dbaston commented Apr 15, 2016

strk commented Apr 15, 2016 via email

dbaston commented Apr 15, 2016

strk commented Apr 16, 2016 via email

dbaston commented Apr 16, 2016

strk commented Apr 17, 2016 via email

dbaston commented Apr 18, 2016

strk commented Apr 19, 2016

dbaston commented Apr 19, 2016

strk commented Apr 19, 2016 via email

dbaston commented Apr 19, 2016

strk commented Apr 19, 2016 via email

strk commented Apr 19, 2016

dbaston commented Apr 19, 2016

strk commented Apr 19, 2016 via email

strk commented Nov 10, 2016

dbaston commented Apr 14, 2016 •

edited